-
Notifications
You must be signed in to change notification settings - Fork 896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change the error message. #2773
Conversation
Welcome @gj199575! It looks like this is your first PR to volcano-sh/volcano 🎉 |
Hello, you are very welcome to participate in community contributions. From your description, you can see the events and error log information that will continue to be output by Volcano. I think it would be better to create an issue and classify similar PRs in a unified way. |
@@ -98,8 +98,8 @@ func (alloc *Action) Execute(ssn *framework.Session) { | |||
allNodes := ssn.NodeList | |||
predicateFn := func(task *api.TaskInfo, node *api.NodeInfo) error { | |||
// Check for Resource Predicate | |||
if !task.InitResreq.LessEqual(node.FutureIdle(), api.Zero) { | |||
return api.NewFitError(task, node, api.NodeResourceFitFailed) | |||
if reason := task.InitResreq.LessEqualResource(node.FutureIdle(), api.Zero); len(reason) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add a new function here? If a return value is added on the basis of the original function, it indicates whether the resource information of the error is acceptable.
In addition, there are many places where lessEqual is called. Do other places need to be modified or modified iteratively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add a new function here?
Because the purpose of this PR is to make volcano event belong to a unscheduled pod , is more similar to kube-scheduler 。
So I do not need to change many codes。
And the LessEqual function is used by many places , if I change LessEqual function , it is a huge project。
So I choose a better way to solve my problem ,and do not change volcano a lot。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are worried that the impact of the modification is too large, you can add a new function, but the return value of the new function is recommended to return two (bool, string), the first parameter indicates the result, and the second parameter indicates msg, in the return value character Whether the string is empty indicates the result of the judgment value, which feels inappropriate
ok, I will create a issue |
Please refer to the prompt information DCO to repair CI |
aee5781
to
3c35cfc
Compare
ok |
dd17d67
to
be8cff3
Compare
/assign @jiangkaihua |
/priority important-soon |
@@ -98,8 +98,8 @@ func (alloc *Action) Execute(ssn *framework.Session) { | |||
allNodes := ssn.NodeList | |||
predicateFn := func(task *api.TaskInfo, node *api.NodeInfo) error { | |||
// Check for Resource Predicate | |||
if !task.InitResreq.LessEqual(node.FutureIdle(), api.Zero) { | |||
return api.NewFitError(task, node, api.NodeResourceFitFailed) | |||
if ok, reason := task.InitResreq.CheckResource(node.FutureIdle(), api.Zero); !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gj199575 LessEqual is an imporment foundamental function. What's the difference between the CheckResource and LessEqual, and why we have to add this new function, Would you provide detail reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gj199575 LessEqual is an imporment foundamental function. What's the difference between the CheckResource and LessEqual, and why we have to add this new function, Would you provide detail reason?
And it is a huge project if I change LessEqual function .So I supply a new function LessEqualWithReason 。 And I will merge LEssEqual and LessEqualWithReason to a funtion 。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
when pod is unscheduled, make the error message similar to kube-scheduler。 Signed-off-by: gj199575 <409237405@qq.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: william-wang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
this MR is the relization of this issue : #2774
when pod is unscheduled, make the error message similar to kube-scheduler。
test message
the default scheduler error message is "0/2 nodes are available: 2 Insufficient cpu."
before this MR
volcano error message is "all nodes are unavailable: 2/2 nodes node(s) resource fit failed."
after this MR
volcano error message is "all nodes are unavailable: 2/2 nodes Insufficient cpu."
And in the future。 I will continue to make volcano error event or something which is showed to user is more similar to k8s default scheduler